Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updates to the help output of command-line options #814

Merged
merged 3 commits into from
Jul 10, 2021
Merged

Updates to the help output of command-line options #814

merged 3 commits into from
Jul 10, 2021

Conversation

jcmoraisjr
Copy link
Owner

A couple of changes to the help output of command-line options and some minor refactors without functionality changes.

  • Improve text alignment and the description of some options
  • Move flag initialization from the controller initialization code to the launch
  • Deprecate --disable-node-list which wasn't being used

@jcmoraisjr
Copy link
Owner Author

#806

toothbrush
toothbrush reviewed Jul 8, 2021
View reviewed changes
pkg/common/ingress/controller/launch.go Outdated
"http://localhost:8080. If not specified, the assumption is that the binary runs inside a "+
"Kubernetes cluster and local discovery is attempted.")
apiserverHost = flags.String("apiserver-host", "",
`The address of the Kubernetes Apiserver to connect to in the format of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`The address of the Kubernetes Apiserver to connect to in the format of
`The address of the Kubernetes API server to connect to, in the format of

I'm a bit pedantic about wording -- feel free to completely ignore me but i thought i'd clean up some of the wording a little, if you're modifying it already!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be my guest =) Because of that I open pull requests and wait a couple of days before merge - this way people has the chance to ping and ask "what?". I'll have a look into the comments shortly.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied all the changes, thanks for the suggestions.

@jcmoraisjr jcmoraisjr force-pushed the jm-cleanup-cmdline branch from 9423793 to fb65b45 Compare July 8, 2021 11:04
@jcmoraisjr
deprecate --disable-node-list command-line option
b2fb397 
`--disable-node-list` is actually not being used by the current
controller version. We're now removing an unused lister and informer
and also deprecating the command-line option for future removal.
@jcmoraisjr
move command-line tuning from controller to launch
b5f9173 
Move all command-line related tunings from the controller initialization
to the launch process. This difference is an inheritance from the
controller as a dependency days (v0.7 and older) and wasn't being
revisited yet. There is no chance in the functionalities.
@jcmoraisjr
docs: reorg help output of command-line options
7507f34
@jcmoraisjr jcmoraisjr force-pushed the jm-cleanup-cmdline branch from fb65b45 to 7507f34 Compare July 10, 2021 18:57
@jcmoraisjr jcmoraisjr merged commit 931f4cf into master Jul 10, 2021
@jcmoraisjr jcmoraisjr deleted the jm-cleanup-cmdline branch July 10, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toothbrush toothbrush toothbrush left review comments

Assignees
No one assigned
Labels
v0.13 v0.14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcmoraisjr @toothbrush